-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Add telemtry tags to queries
#21303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adjusts metadata on several Rust “summary” metric queries so they’re additionally tagged as telemetry, which (per the PR description) reduces what appears in the end-of-scan summary table.
Changes:
- Add the
telemetrytag torust/summary/summary-statistics. - Add the
telemetrytag torust/summary/reduced-summary-statistics. - Add the
telemetrytag torust/summary/query-sink-countsandrust/summary/nodes-at-type-path-length-limit.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rust/ql/src/queries/summary/SummaryStatsReduced.ql | Adds telemetry to the query tags. |
| rust/ql/src/queries/summary/SummaryStats.ql | Adds telemetry to the query tags. |
| rust/ql/src/queries/summary/QuerySinkCounts.ql | Adds telemetry to the query tags. |
| rust/ql/src/queries/summary/NodesWithTypeAtLengthLimit.ql | Adds telemetry to the query tags. |
geoffw0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you show in the PR description looks good. I won't pretend to know for sure all the possibly effects of this tag, the query metadata doc page doesn't shed any light, I suggest we go ahead with the change and just keep an eye on things after it is merged.
| * @kind metric | ||
| * @id rust/summary/summary-statistics | ||
| * @tags summary | ||
| * @tags summary telemetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do the same change to the Swift versions of these queries (that I am generally responsible for) - swift/summary/summary-statistics and swift/summary/query-sinks at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer doing that on a separate PR.
Before this PR, each scan would end with a summary like
Now, it instead ends with
I don't think the removed rows provide any relevant information, hence this PR. I'm unsure, though, whether this will break something in the tool status page?